Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Example of unfriendly closure error messages #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jadski
Copy link

@jadski jadski commented May 20, 2015

@evancz
Copy link
Member

evancz commented May 21, 2015

Can you get this down to a more minimal example? As it is, it requires elm-linear-algebra. Is it possible to reproduce the character of the mistake without any extra packages? Ideally the code would be a small let that still demonstrates the issue.

@evancz
Copy link
Member

evancz commented May 22, 2015

I messed around with this a bit today, and it seems like the trouble is that the conflict arises when you have the type annotation. So the whole expression type checks properly, but the value it gives out does not have the type listed in the annotation.

So it is true that the large expression is the root of the error. Does that make sense?

What behavior would you expect given these facts?

@evancz
Copy link
Member

evancz commented May 22, 2015

I also refactored the code to how I would probably write such a thing:

hacky2 : Dict.Dict String ParsedSvgData -> List Country
hacky2 countryShapes =
  List.map makeCountry (Dict.toList countryShapes)

makeCountry ( id, record ) =
  { code  = id
  , name  = record.title
  , lines = makeLines record.polygons
  }

makeLines polygons =
  case polygons of
    first :: rest ->
        let
          vectors = List.map makeVector first
        in
          List.map2 helper vectors rest ++ makeLines rest

    [] ->
        []

makeVector ( x, y ) =
  ( ( x / scale ), ( y / scale ) )

helper v1 v2 =
  ( { a1 = v1 }, { a2 = v2 } )

And it seems like it is reporting the error message in an entirely appropriate way. Breaking the function up in this way also means that it is much easier to do unit testing on the relevant functions. And add type annotations.

It may be possible to try to push the information from the type annotation down into the expression, but I am not totally sure. That could help with this case maybe...

@evancz
Copy link
Member

evancz commented May 26, 2015

@rtfeldman has shared another example in this category, which is "the type annotation does not match the defined value."

I have a potential improvement that would spit out this error message for you:

-- TYPE MISMATCH ------------------------------------------------------ hats.elm

The type annotation for 'hacky2' does not match its definition.

61| hacky2 countryShapes =
    ^^^^^^
To be more specific, type inference is leading to a conflict between this type:

    Math.Vector2.Vec2

and this type:

    (Float, Float)

@jadski, how does this look?

@jadski
Copy link
Author

jadski commented Sep 14, 2015

@evancz - thanks for this research and feedback.

My example was designed to show-off a non-trivial (not necessarily optimally written) closure error message and the resulting helplessness I felt.

Your code definitely looks easier to manage, and no doubt represents better practice. However I would feel irked if I was forced to adopt your style because the other generates cryptic error messages.

Unless I've missed something, your revised error message example doesn't really simplify diagnosis of the offending clause.

The problem was that the error message did not report with precision which clause in the let was at fault (although the parser presumably knows this information) - I would hope to see something more like this (where makeVector is defined in the let of the hacky2 function)

-- TYPE MISMATCH ------------------------------------------------------ hats.elm
The type annotation for 'makeVector' does not match its definition.
66| makeVector ( x, y ) =
    ^^^^^^
To be more specific, type inference is leading to a conflict between this type:
    Math.Vector2.Vec2
and this type:
    (Float, Float)

I know you've improved error messages since this discussion, so I'll check these out.

@ShalokShalom
Copy link

Is this solved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants